Adds rawBody support to Node.js #2064
Conversation
|
@christopheranderson, |
|
|
|
Which integration tests are you referring to? I see a green run for this PR |
| @@ -252,4 +252,5 @@ message RpcHttp { | |||
| string status_code = 12; | |||
| map<string,string> query = 15; | |||
| bool is_raw = 16; | |||
| TypedData rawBody = 17; | |||
There was a problem hiding this comment.
I think that this will always be a string, so you can concretely type it as a string.
There was a problem hiding this comment.
Unfortunately, it's not always a string, sometimes it's None. I could have created a new type which only has those types, but I opted to reuse the existing TypedData object since it wasn't very opinionated.
There was a problem hiding this comment.
Correct - in those cases, would not setting rawBody be sufficient? Protobuf doesn't require all properties to be set in a message, so I think that all client libs will handle the None case. I'd suggest that this change is scoped only to node, which definitely handles nullables. To enable this behavior for node & other workers in the future we could pass the body bytes directly to the client, where they can determine rawbody
That said, I'm on vacation and TypedData works. Just my 2¢ :)
There was a problem hiding this comment.
If I don't set it, it's null. null !== undefined which is a breaking behavior. protobuf for Node.js doesn't seem to have an option for "null" - looks like a fun thread: protocolbuffers/protobuf#1606
christopheranderson
force-pushed
the
feature/node-rawbody
branch
from
aa1740c
to
9804de1
Compare
| @@ -109,6 +111,7 @@ public static TypedData ToRpc(this object value) | |||
| var bytes = new byte[length]; | |||
| request.Body.Read(bytes, 0, length); | |||
| body = bytes; | |||
| rawBody = bytes.ToString(); | |||
There was a problem hiding this comment.
This will give you the type name. Use Encoding.UTF8.GetString (or appropriate encoding)
There was a problem hiding this comment.
May want to make sure you have a failing test for this before fixing :)
christopheranderson
force-pushed
the
feature/node-rawbody
branch
from
9804de1
to
7da966a
Compare
|
Merged in a97d137 |
(BLOCKED) Requires changes from Azure/azure-functions-nodejs-worker#48Please review the PR above which will unblock this PR by allowing me to pull in the NuGet. Tests will fail until that's in.fixes #1951